Restore sidebar workspace icon size#4973
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes browser-stack sidebar sizing into ExtensionBrowserStackSidebarMetrics and updates tile/row views to use it; aligns header/icon/frame sizing with shared HeaderChromeControlMetrics, adjusts titlebar style presets, removes custom sidebar glyph, and updates tests to assert the new metrics. ChangesBrowser Stack Sidebar Metrics Refactoring
Header & Titlebar Metrics Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 14❌ Failed checks (1 warning, 13 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR restores v0.64.10 visual fidelity across the extension sidebar and titlebar chrome. It centralizes sizing so the sidebar icon tile, list rows, and shared header controls all derive from the same metric roots instead of scattered magic numbers.
Confidence Score: 5/5Safe to merge — changes are scoped to visual layout metrics and hit-region geometry with no auth, data, or networking paths touched. All sizing changes are pure arithmetic on constant structs with no runtime side-effects. The fittingConfig two-pass shrink (buttons first, spacing second) is geometrically correct and verified by the new per-style fitting loop. MinimalModeSidebarControlActionSlot raw-value ordering aligns with sidebarChromeSlots order and is exercised by the hit-region probe tests. The barVerticalPadding formula resolves to 2pt (confirmed by test) and keeps controlHeight equal to headerControlSize. Regression assertions lock every metric changed by the PR, making silent regressions on future refactors unlikely. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[HeaderChromeControlMetrics\nbuttonSize=24 iconSize=15 cornerRadius=8] --> B[RightSidebarChromeMetrics\nbarVerticalPadding derived]
A --> C[TitlebarControlsStyle.classic.config\nbuttonSize=24 iconSize=15 spacing=10]
A --> D[HeaderChromeIconStyle\nweight=.semibold]
C --> E{titlebarControlsSidebarChromeConfig}
E -->|fittingConfig maxWidth=124pt| F[Fitted sidebarConfig\nshrinks if needed]
C --> G[TitlebarControlsAccessoryViewController\nsidebarChromeSlots 3 buttons]
F --> H[HiddenTitlebarSidebarControlsView\nstyleConfigOverride=fittedConfig]
F --> I[MinimalModeSidebarControlActionProxyView\nbuttonCount=3]
F --> J[TitlebarControlsGapDragView\nbuttonCount=3]
F --> K[MinimalModeTitlebarButtonHitRegionView\nbuttonCount=3]
L[ExtensionBrowserStackSidebarMetrics\niconSize=28 tileHeight=54] --> M[VerticalTabsSidebar tile row]
L --> N[VerticalTabsSidebar list row\nrowHorizontalPadding rowVerticalPadding rowCornerRadius]
Reviews (12): Last reviewed commit: "merge: resolve conflicts with main" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
+1 for configurable sidebar icon size — same need on the icon side. The workspace icons are hard to distinguish at the current fixed size when a lot of tabs are open. Would love to see this land alongside sidebar font sizing. |
|
@lugfug I kept this PR scoped to the visual regression: the default workspace sidebar icon size is restored to the v0.64.10-era 28pt size, and the sizing now comes from a shared metrics path so a future preference can drive it cleanly. I did not add a new user-facing icon-size setting here because that changes the settings/config surface and needs row-height/density behavior designed separately from this regression fix. |
Branch had diverged behind main, which reworked the titlebar shortcut-hint layout (#5045 clipping fix, #5059 hint alignment with defaultTitlebarHintX 4->0) and shrank header chrome sizing. Resolution preserves the branch's intent (restored release sizes 24/15/17/8, sidebar.left symbol, 3-slot sidebar chrome button-count plumbing) while adopting main's newer hint-layout machinery: - WindowChromeMetrics: keep restored sizes; add main's titlebarControlsLeadingPadding. - TitlebarControlsHitRegions: keep sidebarChromeButtonCount/allTitlebarButtonCount (referenced widely by auto-merged code); adopt named outerLeadingPadding. - contentSize: combine main's two-reservation clipping fix with the branch's buttonCount/reservesShortcutHintOverflow threading; bound titlebarHintLayoutRightmostExtent to the active slot count for sidebar chrome. - Drop branch's superseded hint positioning (titlebarButtonRightEdge, hintRightSafetyShift) in favor of main's planner (hintInterval/titlebarHintPillWidth). - Update regression anchors to merged behavior (defaultTitlebarHintX=0): classicWithShortcutHints 186->172, sidebar 118->104, roomy fitted spacing 7->14. vendor/bonsplit working tree checked out to the merged pointer (adds isAudioMuted, forkConversation* APIs that Workspace.swift now uses). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 13ee964. Configure here.
| onFocusHistoryForward: focusHistoryForward, | ||
| visibilityMode: .alwaysVisible | ||
| visibilityMode: .alwaysVisible, | ||
| actionSlots: TitlebarShortcutHintActionSlot.sidebarChromeSlots |
There was a problem hiding this comment.
Titlebar accessory drops history
High Severity
TitlebarControlsAccessoryViewController now passes actionSlots: TitlebarShortcutHintActionSlot.sidebarChromeSlots (three controls) into TitlebarControlsView, whose default is all cases. Non-minimal workspace windows therefore lose focus back/forward in the system titlebar, contradicting the stated goal to keep those buttons.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 13ee964. Configure here.
2 participants
Summary
sidebar.left) instead of the newer custom drawn glyphRepro / Root Cause
Used local source diffing against
v0.64.10, plus the user-provided screenshots. The shippedv0.64.10titlebar controls usediconSize: 15,buttonSize: 24,spacing: 10, semibold symbols, and SF Symbolsidebar.leftfor the sidebar toggle. The later focus-history/titlebar chrome refactor shrank classic to compact shared values (12pticons in20ptlanes, regular weight) and replaced the sidebar toggle with a smaller custom glyph.This fix restores the release-era titlebar icon/lane scale at the shared chrome metric root and restores the original SF Symbol path for the sidebar toggle, so the main titlebar controls and right-sidebar header controls stay in sync without a one-off frame override.
No cloud-mac video was recorded.
Testing
Launch
Required command after approval:
CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag fix-sidebar-icons-restore-size --launchSummary by CodeRabbit
Note
Low Risk
Mostly visual layout and hit-region math with broad test coverage; no auth, data, or security paths touched.
Overview
Restores release-era (v0.64.10) titlebar chrome at the shared metric layer: larger classic lanes (15pt icons in 24pt buttons), semibold symbols, updated spacing/sizes across titlebar style presets, and
sidebar.leftinstead of the custom sidebar glyph.Minimal-mode sidebar titlebar is narrowed to three chrome actions (sidebar, notifications, new workspace). Layout, hit-testing, gap-drag, and accessory sizing now take an explicit
buttonCount, usetitlebarControlsSidebarChromeConfigto fit the fixed 124pt host (with optional shortcut-hint overflow only for those slots), andTitlebarControlsViewcan render a subset viaactionSlots/styleConfigOverride.Extension browser-stack sidebar rows/tiles pull dimensions from
ExtensionBrowserStackSidebarMetrics(28pt icons, derived vertical padding with a debug assert).Right-sidebar header chrome aligns with the shared titlebar button/icon sizes instead of smaller one-off values.
Reviewed by Cursor Bugbot for commit 13ee964. Bugbot is set up for automated code reviews on this repo. Configure here.